Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

x509.certificate_managed: always call file.managed even when cert contents haven't changed #56788

Closed
wants to merge 4 commits into from
Closed

x509.certificate_managed: always call file.managed even when cert contents haven't changed #56788

wants to merge 4 commits into from

Conversation

glynnforrest
Copy link
Contributor

What does this PR do?

Fixes a bug I introduced when rewriting x509.certificate.managed.

The state now always calls file.managed, even if the certificate contents don't need to change. This allows the user to change the user / group / mode of an existing certificate.

Both file and certificate changes are properly shown in the state output:

Screen Shot 2020-04-21 at 18 26 33
Screen Shot 2020-04-21 at 18 20 30
Screen Shot 2020-04-21 at 18 27 08

Ping @Ch3LL and @waynew, since you worked on #52935.

What issues does this PR fix or reference?

See #52935 (comment)

Merge requirements satisfied?

  • Tests written/updated

Commits signed with GPG?

Yes

@glynnforrest glynnforrest requested a review from a team as a code owner April 21, 2020 18:39
@ghost ghost requested review from garethgreenaway and removed request for a team April 21, 2020 18:39
@glynnforrest glynnforrest mentioned this pull request Apr 21, 2020
Merged
@hatifnatt
Copy link

hatifnatt commented Apr 22, 2020 •
edited
Loading

First of all I must notice that I'm running Salt 2019.2.3 (Fluorine) on Debian 9, Python 2.7 but this PR is based on master branch.

Salt Version:
           Salt: 2019.2.3

Dependency Versions:
           cffi: Not Installed
       cherrypy: 3.5.0
       dateutil: 2.5.3
      docker-py: Not Installed
          gitdb: 2.0.0
      gitpython: 2.1.1
          ioflo: Not Installed
         Jinja2: 2.9.4
        libgit2: Not Installed
        libnacl: Not Installed
       M2Crypto: 0.24.0
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.4.8
   mysql-python: 1.3.7
      pycparser: Not Installed
       pycrypto: 2.6.1
   pycryptodome: Not Installed
         pygit2: Not Installed
         Python: 2.7.13 (default, Sep 26 2018, 18:42:22)
   python-gnupg: Not Installed
         PyYAML: 3.12
          PyZMQ: 16.0.2
           RAET: Not Installed
          smmap: 2.0.1
        timelib: Not Installed
        Tornado: 4.4.3
            ZMQ: 4.2.1

System Versions:
           dist: debian 9.12
         locale: UTF-8
        machine: x86_64
        release: 4.9.0-12-amd64
         system: Linux
        version: debian 9.12

I run few tests:

  • change key file parameters - ok, changes generated with test=True, file parameters updated after execution
  • change cert file parameters - ok, changes generated with test=True, file parameters updated after execution
  • change cert file parameters and change certificate parameters like subjectAltName - mostly ok, changes generated with test=True but changes in file parameters are not shown only changes in certificate are shown. I.e. file mode changed from 640 to 600 and subjectAltName changed from DNS:salt.domain.tld to DNS:salt.domain.tld, DNS:master.domain.tld ony changes in certificate are shown
          ID: salt_api_cert
    Function: x509.certificate_managed
      Result: None
     Comment: Certificate /etc/pki/api/salt.crt will be created
     Started: 18:19:29.181241
    Duration: 334.043 ms
     Changes:   
              ----------
              Status:
                  ----------
                  New:
                      Certificate will be valid and up to date
                  Old:
                      Certificate properties are different: X509v3 Extensions

but after execution all changes are displayed correctly

Duration: 664.762 ms
 Changes:
          ----------
          Certificate:
              ----------
              New:
              ... snip ...
                      subjectAltName:
                          DNS:salt.domain.tld, DNS:master.domain.tld
                  Old:
              ... snip ...
                      subjectAltName:
                          DNS:salt.domain.tld
              File:
                  ----------
                  diff:
                      ---
                      +++
                      @@ -1,40 +1,40 @@
                       -----BEGIN CERTIFICATE-----
                       ... snip ...
                       -----END CERTIFICATE-----
                  mode:
                      0600
              Status:
                  ----------
                  New:
                      Certificate is valid and up to date
                  Old:
                      Certificate properties are different: X509v3 Extensions

May be it's possible to show File: changes during test run too?

@hatifnatt
Copy link

@glynnforrest what OS, Salt and Python versions you are running? I'm trying to upgrade to Deb 10, and therefore forced to switch to Py3 and it looks like I'm faced with #56556

@sjorge
Copy link
Contributor

sjorge commented Apr 24, 2020

@hatifnatt this got merged into master, I tested the copy of x509 module and state and sadly still have the same behavior as in #56556

@hatifnatt
Copy link

hatifnatt commented Apr 24, 2020 •
edited
Loading

@sjorge this is not merged to master for now, #52935 - is merged. But both of these PRs are not supposed to fix issue from #56556.
That's why I'm trying to clarify what versions @glynnforrest is using.

@glynnforrest glynnforrest mentioned this pull request Apr 24, 2020
Closed
@glynnforrest
Copy link
Contributor Author

Thanks @hatifnatt for trying it out. I've added File: changes to the test output:

Screen Shot 2020-05-23 at 11 18 21
Screen Shot 2020-05-23 at 11 09 20

Note that the diff in test mode won't include the new certificate in it, for security and performance reasons. See my comment here: https://github.com/saltstack/salt/pull/56788/files#diff-5499a295a50d60a761c34f4080e4014bR647

@garethgreenaway @dwoz this is ready now.

@sjorge
Copy link
Contributor

sjorge commented May 23, 2020

Should it include the old one though?

@glynnforrest
Copy link
Contributor Author

glynnforrest commented May 23, 2020 •
edited
Loading

Should it include the old one though?

Not 100% sure what you're asking here. If you mean the diff in the file.managed output - that's how the state has behaved up until now (and the new cert is also shown when test mode is off). We could discuss removing the cert from the output in a different PR but I imagine the SaltStack folks will have something to say about that regarding backwards compatibility.

@glynnforrest
Copy link
Contributor Author

Ping @sagetherage, any chance you could get someone to look at this?

@s0undt3ch s0undt3ch mentioned this pull request Jun 16, 2020
Merged
2 tasks
@s0undt3ch
Copy link
Collaborator

This is getting merged as part of #57688 (cherry-picked)
Closing.

@s0undt3ch s0undt3ch closed this Jun 19, 2020
@s0undt3ch
Copy link
Collaborator

Please state if closing was the wrong move, I can re-open.

@hatifnatt hatifnatt mentioned this pull request Nov 17, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@garethgreenaway garethgreenaway Awaiting requested review from garethgreenaway

Assignees
No one assigned
Labels
has-failing-test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@glynnforrest @hatifnatt @sjorge @s0undt3ch @dwoz